Skip to content

Fix issue that caused database rename to and from saved to fail #222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 15, 2024
Merged

Fix issue that caused database rename to and from saved to fail #222

merged 1 commit into from
Oct 15, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 15, 2024 •
edited
Loading

llvm::sys::fs::rename cannot rename directories becasue it creates a file handle for the file to rename, which doesn't work for directories. Implement a custom rename function that uses MoveFileW on Windows.

Fixes swiftlang/sourcekit-lsp#1750

@ahoppen
Fix issue that caused database rename to and from saved to fail
c73df08 
`llvm::sys::fs::rename` cannot rename directories becasue it creates a file handle for the file to rename, which doesn't work for directories. Implement a custom rename function that uses `MoveFileW` on Windows.
@ahoppen ahoppen requested a review from bnbarham as a code owner October 15, 2024 00:39
@ahoppen ahoppen mentioned this pull request Oct 15, 2024
Closed
@ahoppen
Copy link
Member Author

ahoppen commented Oct 15, 2024

@swift-ci Please test

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 15, 2024
@ahoppen
Enable testIndexShutdown
4a52b9f 
The underlying issue should be fixed by swiftlang/indexstore-db#222.
ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 15, 2024
@ahoppen
Enable testIndexShutdown
925d5fe 
The underlying issue should be fixed by swiftlang/indexstore-db#222.

Fixes swiftlang#1750
rdar://137886502
ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 15, 2024
@ahoppen
Enable testIndexShutdown
43c4201 
The underlying issue should be fixed by swiftlang/indexstore-db#222.

Fixes swiftlang#1750
rdar://137886502
@ahoppen ahoppen mentioned this pull request Oct 15, 2024
Merged
@ahoppen
Copy link
Member Author

ahoppen commented Oct 15, 2024

swiftlang/swift#77015

@swift-ci Please test Windows

ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 15, 2024
@ahoppen
Enable testIndexShutdown
a853d05 
The underlying issue should be fixed by swiftlang/indexstore-db#222.

Fixes swiftlang#1750
rdar://137886502
bnbarham
bnbarham approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty weird but... 🤷

@ahoppen
Copy link
Member Author

ahoppen commented Oct 15, 2024

For the record, llvm::sys::fs::rename tries to create a file handle for the directory using CreateFileW and that fails. From Windows documentation of CreateFileW:

Creates or opens a file or I/O device. The most commonly used I/O devices are as follows: file, file stream, directory,

To open a directory using CreateFile, specify the FILE_FLAG_BACKUP_SEMANTICS flag as part of dwFlagsAndAttributes

FILE_FLAG_BACKUP_SEMANTICS: The file is being opened or created for a backup or restore operation. The system ensures that the calling process overrides file security checks when the process has SE_BACKUP_NAME and SE_RESTORE_NAME privileges. For more information, see Changing Privileges in a Token.

So, looks like CreateFileW can open directories but it’s not really supposed to except for backup purposes and should not be the go-to solution for renaming directories.

@ahoppen ahoppen merged commit b093cdb into swiftlang:main Oct 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IndexTests.testIndexShutdown failing on Windows
2 participants
@ahoppen @bnbarham